Skip to content

feat(FIR-49930): add bulk_insert parameter to executemany for improved INSERT performance #463

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 15 commits into
base: refactor-query-preparation
Choose a base branch
from
Open

feat(FIR-49930): add bulk_insert parameter to executemany for improved INSERT performance #463

wants to merge 15 commits into from

Conversation

devin-ai-integration[bot]
Copy link
Contributor

@devin-ai-integration devin-ai-integration bot commented Oct 9, 2025 •
edited by ptiurin
Loading

Add bulk_insert parameter to executemany() for improved INSERT performance

Summary

Adds a new bulk_insert parameter to the executemany() method in both sync and async cursors. When enabled, this feature concatenates multiple INSERT statements into a single batch request with merge_prepared_statement_batches=true, providing significant performance improvements for large data insertions.

Key Changes:

  • Added bulk_insert parameter (defaults to False for backward compatibility)
  • Validates queries are INSERT statements only
  • Supports both QMARK (?) and FB_NUMERIC ($1, $2) parameter styles
  • Concatenates queries with semicolons for batch execution
  • Comprehensive unit and integration test coverage
  • Updated documentation with usage examples

Review & Testing Checklist for Human

Critical items requiring manual verification:

  • End-to-end database testing: Test bulk insert with real Firebolt database using both parameter styles to verify actual performance improvements and correctness
  • Parameter binding validation: Verify complex data types (dates, decimals, strings with special characters) are correctly bound in both QMARK and FB_NUMERIC modes
  • Error handling verification: Test edge cases including non-INSERT queries, empty parameter lists, malformed queries, and large datasets to ensure proper error messages and cursor state management
  • Security review: Verify query concatenation and parameter handling doesn't introduce SQL injection vulnerabilities, especially with user-provided data

Session Info: Implemented by Devin AI for @ptiurin
Session URL: https://app.devin.ai/sessions/45091a6335424e0798501799fd55a858

@devin-ai-integration Devin.ai Integration
Copy link
Contributor Author

devin-ai-integration bot commented Oct 9, 2025 •
edited by ptiurin
Loading

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment and CI monitoring

@ptiurin ptiurin changed the title feat: add bulk_insert parameter to executemany for improved INSERT performance feat(NoTicket): add bulk_insert parameter to executemany for improved INSERT performance Oct 9, 2025
ptiurin
ptiurin reviewed Oct 10, 2025
View reviewed changes
devin-ai-integration bot added a commit that referenced this pull request Oct 10, 2025
@devin-ai-integration @ptiurin
refactor: simplify bulk_insert validation and reuse existing executio…
2b5bb71 
…n logic

- Replace sqlparse with simple string checks in _validate_bulk_insert_query
- Add extra_params parameter to _build_fb_numeric_query_params for extensibility
- Refactor _executemany_bulk_insert to preprocess and delegate to existing methods
- Use TimeoutController for consistent timeout handling
- Parametrize integration tests to avoid duplication

Addresses PR feedback from ptiurin on #463

Co-Authored-By: petro.tiurin@firebolt.io <petro.tiurin@firebolt.io>
devin-ai-integration bot added a commit that referenced this pull request Oct 10, 2025
@devin-ai-integration @ptiurin
refactor: move imports to top of integration test files
36c9f0a 
- Move firebolt.db and firebolt.async_db imports to top of test files
- Remove local import statements from inside test functions
- Use full module names instead of local aliases

Addresses final PR feedback from ptiurin on #463

Co-Authored-By: petro.tiurin@firebolt.io <petro.tiurin@firebolt.io>
ptiurin
ptiurin reviewed Oct 13, 2025
View reviewed changes
devin-ai-integration bot added a commit that referenced this pull request Oct 13, 2025
@devin-ai-integration @ptiurin
docs: clarify bulk_insert only supports fb_numeric parameter style
49854e4 
- Remove QMARK parameter style example from bulk_insert documentation
- Update note to explicitly state bulk_insert requires fb_numeric
- Clarify that using other parameter styles will raise an error

Addresses PR feedback from ptiurin on #463

Co-Authored-By: petro.tiurin@firebolt.io <petro.tiurin@firebolt.io>
devin-ai-integration bot and others added 12 commits October 14, 2025 14:11
@devin-ai-integration @ptiurin
feat: add bulk_insert parameter to executemany for improved INSERT pe…
967e28a 
…rformance

- Add bulk_insert flag to executemany() in both sync and async cursors
- Concatenate multiple INSERT queries with semicolons when bulk_insert=True
- Send with merge_prepared_statement_batches=true query parameter
- Support both QMARK and FB_NUMERIC parameter styles
- Add validation to ensure only INSERT statements use bulk_insert
- Add comprehensive unit tests for both sync and async cursors
- Add integration tests for V2 (sync and async)
- Add documentation with examples for both parameter styles

Co-Authored-By: petro.tiurin@firebolt.io <petro.tiurin@firebolt.io>
@devin-ai-integration @ptiurin
fix: use sqlparse get_type() for reliable INSERT statement validation
1a098d0 
Co-Authored-By: petro.tiurin@firebolt.io <petro.tiurin@firebolt.io>
@devin-ai-integration @ptiurin
fix: use regex URL matching in bulk_insert tests to handle query para…
2b51b14 
…meters

Co-Authored-By: petro.tiurin@firebolt.io <petro.tiurin@firebolt.io>
@devin-ai-integration @ptiurin
fix: convert URL objects to strings before split in bulk_insert tests
b00d87a 
Co-Authored-By: petro.tiurin@firebolt.io <petro.tiurin@firebolt.io>
@devin-ai-integration @ptiurin
fix: add meta field to bulk_insert test mock responses
9bbb19c 
Co-Authored-By: petro.tiurin@firebolt.io <petro.tiurin@firebolt.io>
@devin-ai-integration @ptiurin
fix: add proper statistics structure to bulk_insert test mock responses
d5e4257 
Co-Authored-By: petro.tiurin@firebolt.io <petro.tiurin@firebolt.io>
@devin-ai-integration @ptiurin
refactor: simplify bulk_insert validation and reuse existing executio…
a3c981b 
…n logic

- Replace sqlparse with simple string checks in _validate_bulk_insert_query
- Add extra_params parameter to _build_fb_numeric_query_params for extensibility
- Refactor _executemany_bulk_insert to preprocess and delegate to existing methods
- Use TimeoutController for consistent timeout handling
- Parametrize integration tests to avoid duplication

Addresses PR feedback from ptiurin on #463

Co-Authored-By: petro.tiurin@firebolt.io <petro.tiurin@firebolt.io>
@devin-ai-integration @ptiurin
refactor: move imports to top of integration test files
eeb4702 
- Move firebolt.db and firebolt.async_db imports to top of test files
- Remove local import statements from inside test functions
- Use full module names instead of local aliases

Addresses final PR feedback from ptiurin on #463

Co-Authored-By: petro.tiurin@firebolt.io <petro.tiurin@firebolt.io>
@ptiurin
only support this in fb_numeric
878ddd2
@ptiurin
refactor for complexity
90d0df0
@devin-ai-integration @ptiurin
docs: clarify bulk_insert only supports fb_numeric parameter style
018b9bb 
- Remove QMARK parameter style example from bulk_insert documentation
- Update note to explicitly state bulk_insert requires fb_numeric
- Clarify that using other parameter styles will raise an error

Addresses PR feedback from ptiurin on #463

Co-Authored-By: petro.tiurin@firebolt.io <petro.tiurin@firebolt.io>
@ptiurin
improve the change and rebase
97b5c56
@ptiurin ptiurin force-pushed the devin/1760003599-bulk-insert-executemany branch from 49854e4 to 97b5c56 Compare October 14, 2025 16:24
@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Passed Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
97.3% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@ptiurin ptiurin changed the base branch from main to refactor-query-preparation October 14, 2025 16:32
@ptiurin ptiurin changed the title feat(NoTicket): add bulk_insert parameter to executemany for improved INSERT performance feat(FIR-49930): add bulk_insert parameter to executemany for improved INSERT performance Oct 15, 2025
@ptiurin ptiurin marked this pull request as ready for review October 15, 2025 16:39
@ptiurin ptiurin requested a review from a team as a code owner October 15, 2025 16:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ptiurin ptiurin ptiurin left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@ptiurin